feat: Add support for CommitDiff to efficiently commit small changes in large delegated accounts
#575
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughAdds AccountFetcher and an async CommitTask constructor that may fetch a base account and choose commit-diff vs commit-state at runtime; introduces CommittedAccount; adds zero‑copy OrderBook and order-book instructions/handlers; threads per-context user_seed through client/tests; updates manifests and many tests; bumps delegation-program rev. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CommitTask
participant AccountFetcher
participant DelegationProgram
Caller->>CommitTask: await CommitTask::new(commit_id, allow_undelegation, committed_account, account_fetcher)
CommitTask->>CommitTask: inspect committed_account.account.data.len()
alt needs base account (len > THRESHOLD && !force_commit_state)
CommitTask->>AccountFetcher: fetch_account(pubkey) (async)
AccountFetcher-->>CommitTask: Account (fetched)
CommitTask->>CommitTask: store base_account
else
CommitTask->>CommitTask: proceed without base_account
end
Caller->>CommitTask: create_commit_ix(validator)
alt is_commit_diff()
CommitTask->>CommitTask: create_commit_diff_ix(...)
CommitTask->>DelegationProgram: Instruction::CommitDiff(...)
else
CommitTask->>CommitTask: create_commit_state_ix(...)
CommitTask->>DelegationProgram: Instruction::CommitState(...)
end
CommitTask-->>Caller: Instruction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
snawaz
force-pushed
the
snawaz/commit-diff
branch
6 times, most recently
from
249b1aa to
2f0555f
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
0151da0 to
5c25e73
Compare
snawaz
changed the title
MagicBlockInstruction::ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
changed the title
MagicBlockInstruction::ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
2ae750b to
725f72c
Compare
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
725f72c to
e5c5e15
Compare
snawaz
changed the title
ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
snawaz
changed the title
ScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accountsScheduleCommitDiffAndUndelegate to efficiently commit changes in delegated accounts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/args_task.rs (1)
229-237: reset_commit_id ignores CommitDiff—update both variants.Commit IDs must update for Commit and CommitDiff.
Apply:
- let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - log::error!("reset_commit_id"); - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => {} + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(6 hunks)magicblock-committor-service/src/tasks/mod.rs(1 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(1 hunks)magicblock-rpc-client/src/lib.rs(1 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(4 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(5 hunks)test-integration/Cargo.toml(2 hunks)test-integration/programs/schedulecommit-security/src/lib.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
PR: magicblock-labs/magicblock-validator#585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (15)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (3)
test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_with_payer_cpi_instruction(214-232)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
assert_two_committees_were_committed(68-91)get_context_with_delegated_committees(16-35)assert_feepayer_was_committed(94-113)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_commit_result_from_logs(7-14)
test-integration/programs/schedulecommit-security/src/lib.rs (1)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)
magicblock-committor-service/src/tasks/args_task.rs (4)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/config.rs (1)
local(29-35)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/magicblock_processor.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (1)
instruction(58-160)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-259)magicblock-committor-program/src/state/changeset.rs (1)
request_undelegation(230-232)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (4)
schedule_commit_and_undelegate_cpi_instruction(234-252)schedule_commit_and_undelegate_cpi_with_mod_after_instruction(287-311)schedule_commit_diff_instruction_for_order_book(193-212)update_order_book_instruction(175-191)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)assert_one_committee_account_was_undelegated_on_chain(190-196)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
committees(205-209)ephem_blockhash(322-324)ephem_client(318-320)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)magicblock-committor-service/src/tasks/args_task.rs (1)
new(49-54)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (34)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
55-55: LGTM! Correctly updated to match new function signature.Both call sites properly destructure the new 3-tuple return value. The
_tmpdirbinding is necessary to keep theTempDiralive throughout the function scope, preventing premature cleanup of the temporary directory while the validator is still using it.Also applies to: 170-170
test-integration/programs/schedulecommit/src/utils/mod.rs (2)
53-53: LGTM: Type alignment with Solana API.Changing
sizetou64aligns with Solana'ssystem_instruction::allocateAPI, which expects au64parameter. This improves type consistency and eliminates the need for type conversions downstream.
70-90: LGTM: Clean implementation with helpful debugging.The changes correctly handle the type conversion:
- Line 71: The cast to
usizeis necessary forRent::minimum_balanceand is safe given Solana's practical account size limits.- Lines 75-80: The debug log provides useful visibility into lamport requirements during account allocation.
- Line 90: Passing
sizedirectly asu64tosystem_instruction::allocateis cleaner and aligns with the API signature.test-integration/programs/schedulecommit-security/src/lib.rs (2)
2-2: LGTM: Import addition is correct.The
CommitPolicyimport is necessary for the API change on line 149.
143-150: No action required—the two CPI paths use intentionally different APIs.The review comment's concern about consistency is based on a false premise. The code uses two fundamentally different functions from different modules:
- Path 1 (
schedule_commit_cpi_instruction): A local program API wrapper with a fixed, simplified interface- Path 2 (
create_schedule_commit_ix): An external SDK function with extended configuration options includingCommitPolicyThese are not parallel implementations requiring symmetry. The
schedule_commit_cpi_instructionfunction does not supportCommitPolicyby design—it wraps commit scheduling with hardcoded policy defaults. AddingCommitPolicy::UseFullBytestocreate_schedule_commit_ixis the correct change and requires no corresponding update toschedule_commit_cpi_instruction.Likely an incorrect or invalid review comment.
test-integration/test-tools/src/integration_test_context.rs (1)
167-171: Transaction version configuration is correct and environment-aware.The conditional logic properly handles two distinct RPC endpoints:
- Chain (Solana devnet):
max_supported_transaction_version: Some(0)with 50 retries to handle devnet's known quirk of sometimes returning version responses instead of transaction metadata (noted at line 160-161).- Ephemeral (local validator):
Noneto use default behavior on a controlled, local environment without such quirks.Git history confirms this change is part of the current PR (commit 4c75d78). The code is correctly differentiated for each environment. No changes needed.
test-integration/test-tools/src/scheduled_commits.rs (1)
182-213: LGTM! Clear hierarchical naming for log variables.The renaming from
ephem_logs/chain_logstoephem_logs_l1/ephem_logs_l2improves clarity by establishing a consistent naming convention that reflects the two-level log retrieval hierarchy.magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (1)
29-33: LGTM: CommitDiff included in strategy persistence.Cleanly broadens Commit to CommitDiff without changing strategy semantics.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
224-230: Diff-aware commit routing is correct; keep logs concise.Choosing StandaloneDiff when request_diff is true is right. Consider demoting the "StandaloneDiff" ic_msg to debug in production builds to reduce log noise.
Also applies to: 233-238
29-32: All call sites properly initialize request_diff—no corrections needed.Verification confirms that every
process_schedule_commit()invocation explicitly sets bothrequest_undelegationandrequest_difffields. All three instruction paths (ScheduleCommit,ScheduleCommitAndUndelegate,ScheduleCommitDiffAndUndelegate) pass values forrequest_diff, withScheduleCommitDiffAndUndelegatecorrectly setting it totrueto trigger theCommitType::StandaloneDiffpath. No uninitialized defaults are used, and no silent routing to full-state commits can occur.programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs (1)
29-30: LGTM: commit_diff added end-to-end with safe logging.Field is carried into printable form and logged only as a flag. Tests initialize the new field.
Also applies to: 44-45, 71-73, 215-217, 254-255
magicblock-committor-service/src/tasks/args_task.rs (1)
197-205: Calibrate compute units for CommitDiff based on diff size.Fixed 65_000 may be inaccurate. Once diff is precomputed, scale CU by diff length/segments or add headroom to avoid CU errors.
Based on learnings
programs/magicblock/src/magicblock_processor.rs (3)
2-2: LGTM: import added is correct.
51-54: ScheduleCommit: explicit request_diff=false is correct.
90-99: New ScheduleCommitDiffAndUndelegate path correctly flips request_diff=true.magicblock-committor-service/src/tasks/task_builder.rs (1)
96-105: Per-account Commit vs CommitDiff task selection is correct.programs/magicblock/src/magic_scheduled_base_intent.rs (6)
104-107: ScheduledBaseIntent::is_commit_diff added — OK.
155-164: MagicBaseIntent::is_commit_diff added — OK.
448-454: CommitType::is_commit_diff implementation — OK.
456-463: Getters updated for StandaloneDiff — OK.Also applies to: 465-473
475-487: is_empty handles StandaloneDiff — OK.
317-323: StandaloneDiff variant already exists and is actively used in the codebase.The review's concern assumes
StandaloneDiffis a new addition that will shift bincode discriminants. However, the variant is already present inprocess_schedule_commit.rs(lines 225–226), indicating it exists before this PR or is not new.If
StandaloneDiffwas already committed to the codebase:
- The bincode discriminant shift already happened (or never was a concern).
- The review comment's warning is outdated or misguided.
If the snippet shows the final state (post-changes) and
StandaloneDiffwas genuinely added in this PR:
- Verify the diff shows it was inserted between existing variants (which would shift discriminants).
- If it was appended after existing variants, discriminants remain stable and no migration is needed.
Critical finding: No backwards compatibility tests, no versioning code, and no serde external tagging exist. If persisted state truly exists and the variant order changed, this is a risk. However, the evidence suggests
StandaloneDiffis not new.Action for developer: Confirm in the PR diff that you are not reordering existing enum variants. If appending new variants after existing ones, bincode compatibility is safe. If inserting between existing variants, migration is required.
test-integration/schedulecommit/test-scenarios/Cargo.toml (1)
19-20: LGTM!The addition of
randandborshas workspace dev-dependencies appropriately supports the new order-book test scenarios introduced in this PR.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
30-31: LGTM!The addition of deterministic seed
b"magic_schedule_commit"improves test reproducibility by ensuring consistent PDA derivation across test runs.test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (2)
16-35: LGTM!The seed parameter enables deterministic test context initialization, and explicitly driving the
init_committeesanddelegate_committeesworkflow steps with debug logging improves test clarity and observability.
41-47: LGTM!Making
assert_one_committee_was_committedgeneric over typeTwith appropriate trait bounds enables reuse across different account types (MainAccount, OrderBookOwned, etc.) while maintaining type safety.test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (2)
23-80: LGTM!The test properly validates that attempting to commit a fee payer without escrowing lamports fails with the expected "DoesNotHaveEscrowAccount" error. The test structure and assertions are appropriate.
83-135: LGTM!The test correctly validates the happy path where fee payer commits succeed when lamports are properly escrowed. The verification flow and assertions comprehensively check that both committees and the fee payer were committed and synchronized.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (3)
55-56: LGTM!The addition of deterministic seed
b"magic_schedule_commit"ensures reproducible test behavior across runs.
110-164: LGTM!The
commit_and_undelegate_order_book_accounthelper follows the established pattern of other commit helpers in this file, properly constructing the update and schedule-commit-diff instructions sequence.
244-312: Excellent reproducibility design for randomized testing.The test demonstrates best practices for randomized testing:
- Prints the RNG seed for reproducibility
- Includes the seed in assertion failure messages
- Uses seeded RNG (StdRng) for deterministic behavior given a seed
This allows failures to be reproduced by rerunning with the printed seed value.
One optional enhancement: consider adding a way to override the seed via environment variable for easier failure reproduction:
let rng_seed = std::env::var("TEST_RNG_SEED") .ok() .and_then(|s| s.parse::<u64>().ok()) .unwrap_or_else(|| OsRng.next_u64());test-integration/programs/schedulecommit/src/lib.rs (1)
561-571: Use commit_diff for undelegate path; this looks correct.The diff-based commit is invoked when
undelegateis true; good alignment with the new optimization.Please confirm tests cover both diff and full-commit paths (with/without
undelegate).test-integration/programs/schedulecommit/src/order_book.rs (1)
101-116: Bounds/capacity logic is good; but relies on header being valid.Once header init is fixed in
process_init_order_book, these helpers are fine for basic growth semantics.Ensure tests cover:
- Non-zero existing asks/bids followed by additional inserts (to exercise prefix/suffix behavior).
- Capacity exhaustion returning
None.Also applies to: 118-138, 163-184
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
260-267: Seed semantics are correct and consistently applied across the codebase.The verification shows that
delegate_account_cpi_instruction()explicitly branches on two supported canonical seeds:
b"magic_schedule_commit"→ScheduleCommitInstruction::DelegateCpib"order_book"→ScheduleCommitInstruction::DelegateOrderBookAll test utilities and callers supply only these supported seeds. The
delegate_committees()function at lines 260-267 correctly passes&self.user_seed, and all constructors receive only canonical seeds from test callers. PDA derivation and instruction creation use consistent seed semantics.
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
9c44347 to
693af7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (22)
magicblock-rpc-client/src/lib.rs (1)
430-430: Redundant error prefix persists in log message.The
error!macro already includes ERROR level in its output. The explicit"> ERROR:"prefix creates redundancy and reduces log readability.Apply this diff to remove the redundant prefix:
- error!("> ERROR: {:?}", err); + error!("{:?}", err);Or add context instead:
- error!("> ERROR: {:?}", err); + error!("Transaction processed status failed: {:?}", err);test-integration/test-tools/src/scheduled_commits.rs (2)
198-210: Verbose debug output concern still applies.The past review comment regarding verbose debug output from
println!with{:#?}formatting remains applicable to these new debug statements. Consider making the debug output conditional or using a more compact format.
233-235: Unnecessary clone() on Copy type.The past review comment regarding the redundant
.clone()call onSignature(which implementsCopy) remains applicable. Use*siginstead ofsig.clone().test-integration/test-tools/src/integration_test_context.rs (1)
157-158: Format the panic message withlabel.The panic still prints the literal
rpc_client for [{}] does not exist, so the label never appears when this trips. Please injectlabelinto the message.- let rpc_client = - rpc_client.expect("rpc_client for [{}] does not exist"); + let rpc_client = rpc_client.unwrap_or_else(|| { + panic!("rpc_client for [{}] does not exist", label) + });test-integration/schedulecommit/client/src/verify.rs (1)
16-23: Code duplication: consider the generic implementation suggested in the previous review.This function duplicates
fetch_and_verify_commit_result_from_logsexcept for the type parameter. A generic implementation withT: borsh::BorshDeserializewould eliminate this duplication.test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
115-118: PDA derivation must match program-side seeds.The PDA is derived using an arbitrary
user_seed, which may not match the on-chain program's expected seeds (e.g.,[b"order_book", book_manager]or[b"magic_schedule_commit", player]). This mismatch will cause transaction failures.
170-203: Validate user_seed explicitly to prevent silent misconfiguration.The implicit
elsebranch will execute for anyuser_seedthat isn'tb"magic_schedule_commit", including invalid values. Add explicit validation to fail fast with a clear error when an unsupported seed is provided.test-integration/programs/schedulecommit/src/api.rs (1)
106-109: Derive delegated PDA with canonical seeds to match on-chain expectations.The PDA is derived using an arbitrary
user_seed, but the on-chain program expects fixed seeds like[b"magic_schedule_commit", player]or[b"order_book", book_manager]. This mismatch will cause thedelegated_accountto not match program-side expectations.Cargo.toml (1)
112-112: CI/CD will fail: local path dependency not available.The path
../delegation-programdoes not exist in the repository or CI environments, causing all builds to fail. Either:
- Revert to a git-based dependency (consistent with the learning about using git branch references)
- Make
delegation-programa workspace member at the correct path- Update CI workflows to checkout the dependency
Verify the intended dependency resolution strategy:
#!/bin/bash # Check if the delegation-program path exists if [ -d "../delegation-program" ]; then echo "✓ Path exists locally" ls -la ../delegation-program/ else echo "✗ Path ../delegation-program does not exist" fi # Check for any git submodule configuration echo "" echo "=== Git submodules ===" git submodule status 2>/dev/null || echo "No submodules configured" # Check recent commits for this dependency change echo "" echo "=== Recent changes to delegation-program dependency ===" git log -n 5 --oneline --all -- Cargo.toml | head -10test-integration/Cargo.toml (1)
40-40: Critical: Local path dependencies will break CI builds.This issue has already been flagged in previous review comments. The local paths
../../ephemeral-rollups-sdk/rust/sdkand../../delegation-programdo not exist in the CI environment and will cause build failures.Also applies to: 60-60
test-integration/programs/schedulecommit/src/lib.rs (4)
302-309: Zero the order-book header immediately after allocation.
OrderBook::newreadsbids_len/asks_lenstraight from account data. Leaving those bytes uninitialized means random counts, which makes the next borrow calculate bogus slices and can walk past the buffer. Please clear the header bytes right after allocation (e.g. borrow data, zero the firstsize_of::<OrderBookHeader>()bytes).Apply this diff:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + { + let mut data = order_book.try_borrow_mut_data()?; + let header_len = core::mem::size_of::<OrderBookHeader>(); + if data.len() < header_len { + return Err(ProgramError::AccountDataTooSmall); + } + for byte in &mut data[..header_len] { + *byte = 0; + } + } + Ok(())
329-334: Fix the PDA assertion message.The diagnostic still prints
payer.key, so a failing check points at the wrong seed. Swap it forbook_manager.keyas previously requested.Apply this diff:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
398-403: Update the log label.This path is mutating an existing order book, not initializing one. Please change the
msg!to say “Update order book” (or equivalent) so logs stay accurate.
424-451: Use the right error kind and require the payer to sign.When the accounts slice is the wrong length you must emit
ProgramError::NotEnoughAccountKeys;MissingRequiredSignatureis misleading. More importantly, we still never verifypayer.is_signer, so the CPI can run with an unsigned funder. Addassert_is_signer(payer, "payer")?before the CPI.Apply this diff:
let [payer, order_book_account, magic_context, magic_program] = accounts else { - return Err(ProgramError::MissingRequiredSignature); + return Err(ProgramError::NotEnoughAccountKeys); }; + assert_is_signer(payer, "payer")?; + commit_diff_and_undelegate_accounts( payer, vec![order_book_account], magic_context, magic_program,test-integration/programs/schedulecommit/src/order_book.rs (3)
45-55:deserializestill aliases immutable data mutably (UB).
slice::from_raw_parts_mutonbook_bytesforges a&mut [u8]from shared data, violating Rust’s aliasing rules and instantly triggering undefined behaviour. Please replace this with a safe path—e.g. copy into an owned buffer or implement a parser that reads header/levels straight from the immutable slice.
85-93:OrderBook::newassumes alignment that account data does not guarantee.Casting
header_bytes.as_ptr()to*mut OrderBookHeader(and the level pointer to*mut OrderLevel) requires 4/8‑byte alignment, but Solana account data is byte-aligned. The moment you dereference those pointers you trigger UB. Rework this to treat the buffer as raw bytes and read/write fields withfrom_le_bytes/to_le_bytesinstead of typed references.
140-178: Typed slices over account data remain unsafe.
slice::from_raw_parts(_ as *mut OrderLevel, ...)for bids/asks/buffer helpers creates slices ofOrderLevelout of unaligned memory. That’s the same UB called out earlier. Please keep the backing store as[u8], then decode/encode eachOrderLevelby copying 16 bytes into a local array and usingu64::from_le_bytes/u64::to_le_bytes.magicblock-magic-program-api/src/instruction.rs (1)
111-111: Add documentation for ScheduleCommitDiffAndUndelegate.This issue was already identified in a previous review. The variant lacks documentation while all sibling variants include doc comments describing the instruction purpose and account references.
magicblock-committor-service/src/tasks/args_task.rs (3)
75-103: Stop hard-coding the RPC endpoint in CommitDiff.
ChainConfig::local(ComputeBudgetConfig::new(1_000_000))forces every CommitDiff task to dialhttp://localhost:7799with Processed commitment and re-create an RPC client each timeinstruction()is called. In production that URI does not exist, so we immediately fall back tocommit_stateand never ship a diff; plus every call repeats the network fetch we already warned about. Inject the realChainConfig/RpcClientfrom the service (or precompute/cache the diff during task construction) soinstruction()stays pure and uses the configured endpoint. Prior feedback on this remains unresolved.
115-116: Do not log raw diff payloads.Dumping the entire diff at warn level leaks account data and explodes log volume. Log only bounded metadata (e.g., commit_id, pubkey, diff length) instead.
- log::warn!("DIFF computed: {:?}", args.diff); + log::debug!( + "commit_diff computed: commit_id={} pubkey={} len={}", + value.commit_id, + value.committed_account.pubkey, + args.diff.len() + );
171-173: Remove the CommitDiff panic in optimize().
optimize()is part of the normal pipeline; panicking here will abort the worker whenever a CommitDiff task flows through. ReturnErr(self)(mirroring the other variants) so the caller can fall back cleanly.- ArgsTaskType::CommitDiff(_) => { - panic!("ArgsTaskType::CommitDiff not handled") - } + ArgsTaskType::CommitDiff(_) => Err(self),programs/magicblock/src/magicblock_processor.rs (1)
38-38: Avoid logging entire MagicBlockInstruction payloads.
ic_msg!(..., "{:?}", instruction)dumps the full enum—including the diff bytes you just added—on every invocation. That inflates CU cost and spams logs, an issue we already called out earlier. Drop the log or gate it behind a debug-only feature flag so mainnet builds don’t emit the payload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.toml(1 hunks)magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)magicblock-committor-service/src/tasks/args_task.rs(6 hunks)magicblock-committor-service/src/tasks/mod.rs(1 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(1 hunks)magicblock-rpc-client/src/lib.rs(1 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(4 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_scheduled_commit_sent.rs(5 hunks)test-integration/Cargo.toml(2 hunks)test-integration/programs/schedulecommit-security/src/lib.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-tools/src/integration_test_context.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/test-ledger-restore/tests/08_commit_update.rstest-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/mod.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (15)
test-integration/programs/schedulecommit-security/src/lib.rs (1)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
new(49-54)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)
programs/magicblock/src/magicblock_processor.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (1)
instruction(58-160)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-259)magicblock-committor-program/src/state/changeset.rs (1)
request_undelegation(230-232)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (1)
instruction(65-86)magicblock-committor-service/src/config.rs (1)
local(29-35)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(145-146)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)test-integration/test-tools/src/integration_test_context.rs (1)
try_new(113-115)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (4)
test-integration/test-runner/bin/run_tests.rs (1)
run_test(777-796)test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_with_payer_cpi_instruction(214-232)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (4)
assert_two_committees_synchronized_count(139-169)assert_two_committees_were_committed(68-91)get_context_with_delegated_committees(16-35)assert_feepayer_was_committed(94-113)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_commit_result_from_logs(7-14)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(85-94)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (3)
schedule_commit_and_undelegate_cpi_with_mod_after_instruction(287-311)schedule_commit_diff_instruction_for_order_book(193-212)update_order_book_instruction(175-191)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
get_context_with_delegated_committees(16-35)assert_one_committee_was_committed(41-65)assert_one_committee_account_was_undelegated_on_chain(190-196)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
committees(205-209)ephem_blockhash(322-324)ephem_client(318-320)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
🔇 Additional comments (20)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
49-55: LGTM: Type alignment with Solana API.Changing
sizefromusizetou64correctly aligns with Solana'ssystem_instruction::allocatesignature, which expectsspace: u64.
75-79: LGTM: Helpful debug logging.The added logging for lamport requirements and payer balance aids in troubleshooting account allocation issues during testing.
90-90: Critical fix: Correct system_instruction::allocate signature.The updated call now correctly provides both required arguments (
pubkeyandspace) tosystem_instruction::allocate. The previous implementation appears to have been missing the pubkey argument, which would not match the function signature:pub fn allocate(pubkey: &Pubkey, space: u64) -> Instruction.test-integration/test-tools/src/scheduled_commits.rs (1)
182-213: Variable renaming improves clarity.The renaming of log variables to
ephem_logs_l1andephem_logs_l2clearly distinguishes the two-level log fetching process, and the updated error context at line 206 now correctly references the first-level logs. These changes enhance code readability.test-integration/programs/schedulecommit-security/src/lib.rs (2)
2-2: LGTM!The
CommitPolicyimport is correctly added to support the new argument at line 149.
143-150: Ignore this review comment. The original comment incorrectly assumes thatschedule_commit_cpi_instructionandcreate_schedule_commit_ixshould be consistent, but they are two distinct functions from different crates with different purposes:
schedule_commit_cpi_instruction(fromprogram_schedulecommit::api) is a local CPI helper that does not accept aCommitPolicyparametercreate_schedule_commit_ix(fromephemeral_rollups_sdk::ephem) is an SDK function that acceptsCommitPolicyas a parameterThe two functions have intentionally different signatures. There is no test inconsistency to address.
Likely an incorrect or invalid review comment.
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
55-61: No issues found—code is correct as written.The
TempDirreturned bysetup_validator_with_local_remoteis created specifically for temporary config storage during validator initialization (written instart_magicblock_validator_with_config_structat line 225:fs::write(&config_path, config_toml)). After the validator process is spawned, this directory is no longer needed and is safely dropped. The actual ledger directory is managed separately by the main_tmpdirat line 42 of the test, which persists for the entire test duration. The binding to_tmpdirwith an underscore prefix correctly signals that this return value is intentionally unused beyond validator initialization.test-integration/programs/schedulecommit/src/api.rs (3)
36-54: LGTM!The
init_order_book_instructionfollows the standard instruction builder pattern with appropriate account metadata.
56-75: LGTM!The
grow_order_book_instructioncorrectly constructs the instruction with theadditional_spaceparameter.
175-212: LGTM!Both
update_order_book_instructionandschedule_commit_diff_instruction_for_order_bookfollow correct instruction builder patterns with appropriate account metadata.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
30-31: LGTM! Deterministic seed ensures reproducible test contexts.The hard-coded seed
b"magic_schedule_commit"provides deterministic PDA derivation and context initialization, which improves test reliability and debugging.Also applies to: 84-85
test-integration/schedulecommit/test-scenarios/tests/03_commits_fee_payer.rs (2)
82-135: LGTM! Fee payer escrow test properly verifies expected behavior.The test correctly verifies that with proper escrow setup, the fee payer commit succeeds and all expected assertions pass.
34-41: Fix struct field destructuring:payer_ephemfield andephem_blockhashretrieval.The code attempts to destructure non-existent struct fields:
- Line 35:
payerfield doesn't exist inScheduleCommitTestContextFields— should bepayer_ephem: payer- Line 39:
ephem_blockhashis not a struct field — must be fetched separately viaephem_client.get_latest_blockhash().unwrap()- Lines 58, 112: Remove dereference (
*) fromephem_blockhashonce correctedSee
01_commits.rs(lines 34, 54) for the correct pattern.Also applies to: 88-95, 112
⛔ Skipped due to learnings
Learnt from: bmuddha Repo: magicblock-labs/magicblock-validator PR: 578 File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27 Timestamp: 2025-10-21T14:00:54.642Z Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579programs/magicblock/src/magic_scheduled_base_intent.rs (3)
104-106: LGTM!The
is_commit_diff()method correctly delegates to the base intent, following the same pattern as the existingis_undelegate()method.
155-163: LGTM!The
is_commit_diff()method correctly handles allMagicBaseIntentvariants, properly delegating to the underlyingCommitTypefor bothCommitandCommitAndUndelegatecases.
458-458: LGTM!The accessor methods correctly handle the new
StandaloneDiffvariant, treating it identically toStandalonesince both containVec<CommittedAccount>.Also applies to: 468-468, 480-482
magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)
29-33: LGTM!The match correctly handles both
CommitandCommitDiffvariants identically, as both containCommitTaskand should be persisted the same way.
59-59: ****The review comment is incorrect.
BufferTaskTypeintentionally has only a singleCommitvariant because it represents buffer-based commit operations.CommitDiffis a separate task variant that belongs exclusively toArgsTaskTypeand is handled through a different code path.Evidence:
- ArgsTaskType includes CommitDiff(CommitTask), but BufferTaskType does not
- ArgsTaskType::CommitDiff explicitly panics with "ArgsTaskType::CommitDiff not handled" when attempting conversion, indicating CommitDiff is intentionally not converted to BufferTask
- The refutable pattern at line 59 is safe because BufferTaskType can only ever contain a single variant
No code changes are needed; the refutable pattern is correct as implemented.
Likely an incorrect or invalid review comment.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
31-31: LGTM!The
request_difffield addition cleanly extends the options structure to control diff-based commit scheduling.
231-238: LGTM!The
commit_actionis correctly used in both the undelegation and non-undelegation paths, enabling diff-based commits for both scenarios while preserving existing semantics.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Outdated
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
693af7f to
99f2e8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
test-integration/Cargo.toml (1)
40-40:⚠️ Path dependency concerns from prior review remain unresolved.The migration from git-based to local path dependencies for
ephemeral-rollups-sdkandmagicblock-delegation-programstill poses the same critical CI/CD risks flagged in the previous review: these paths do not exist in the repository, CI workflows do not clone them, and builds will fail in CI/CD and for contributors without manual setup.The prior review's recommendations still apply:
- Update CI to clone both external repositories, OR
- Switch back to git-based references, OR
- Add clear setup documentation with exact clone commands and relative paths
Also applies to: 60-60
magicblock-committor-service/src/tasks/args_task.rs (2)
228-234:reset_commit_idmust handleCommitDiffvariant.The current implementation only updates the
commit_idforCommittasks and silently returns forCommitDiff. When aCommitDifftask is recycled, the stalecommit_idwill remain, causing desynced submissions.Apply this fix to handle both variants:
fn reset_commit_id(&mut self, commit_id: u64) { - // TODO (snawaz): handle CommitDiff as well? what is it about? - let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => { + // Only Commit and CommitDiff tasks have commit_id + } + } }
74-122: Network I/O insideinstruction()remains a major concern.The
instruction()method performs RPC calls and diff computation on every invocation, which is expensive and risky:
instruction()may be called multiple times (e.g., viainvolved_accounts), multiplying RPC calls- TOCTOU: base-chain account can change between diff calculation and transaction submission
- Coupling to network hinders testing and determinism
Per previous discussion, this is acknowledged as temporary and will be addressed in subsequent PRs. Consider tracking this technical debt.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
224-228: Consider enhancing observability as previously suggested.The conditional logic correctly selects the commit type based on
request_diff. However, a past review comment suggests improving the log messages to include the number of committed accounts and the flag value for better operational visibility.test-integration/programs/schedulecommit/src/order_book.rs (2)
99-119: Unaligned typed reference toOrderBookHeaderremains a critical issue.Line 114 creates a typed reference
&mut OrderBookHeadervia raw pointer cast. While alignment is asserted at runtime (lines 102-110), Solana account data does not guarantee alignment for arbitrary types. This is undefined behavior if the account data is not aligned toalign_of::<OrderBookHeader>().The assertion will catch misalignment at runtime, but the proper fix is to avoid typed references entirely. Consider reading/writing the header fields as raw bytes using little-endian encoding.
165-178: Typed slices over potentially unaligned memory (OrderLevel).Lines 166 and 173-174 use
slice::from_raw_partsto create typed slices ofOrderLevel(which containsu64fields requiring 8-byte alignment). Account data alignment is not guaranteed by Solana, leading to undefined behavior on read.Per previous review: store levels as raw bytes and provide accessors that read/write fields via
u64::from_le_bytes/to_le_bytes, or use a packed representation without taking references to fields.test-integration/programs/schedulecommit/src/lib.rs (2)
280-311: Critical: OrderBook header is never initialized.After
allocate_account_and_assign_ownerreturns (line 308), the account data contains uninitialized memory. Subsequent calls toOrderBook::newwill read uninitializedbids_lenandasks_lenvalues, causing undefined behavior.Apply this fix immediately after allocation:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + let header_size = core::mem::size_of::<OrderBookHeader>(); + for b in &mut data[..header_size] { + *b = 0; + } Ok(())
329-334: Incorrect identifier in PDA assertion error message.Line 332 incorrectly references
payer.keyin the error message, which should bebook_manager.keyto accurately reflect the PDA derivation.Apply this fix:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
magicblock-committor-service/src/tasks/args_task.rs(6 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/order_book.rstest-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (1)
instruction(65-86)magicblock-committor-service/src/config.rs (1)
local(29-35)
programs/magicblock/src/magicblock_processor.rs (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-258)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(99-119)
🔇 Additional comments (11)
test-integration/Cargo.toml (1)
72-72: Now let me check the codebase for actual usage of these dependencies to confirm they are necessary:Based on my verification, I can now generate the rewritten review comment:
Both dependencies are compatible with Solana 2.2 and appropriate for this PR.
rkyv 0.7.45 supports no_std and no_alloc configurations, making it suitable for Solana on-chain programs. It supports no-std builds for targets without a standard library and no-alloc builds for targets where allocations cannot be made. The Solana VM guarantees that account data is byte-aligned to a multiple of 8, which enables rkyv's zero-copy deserialization to function safely on account data. static_assertions 1.1.0 is a stable, widely-adopted crate (225,914,390 downloads all time) providing compile-time assertion macros useful for validating account structure layouts and sizes—a common pattern in Solana programs.
No further verification needed.
test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: LGTM! Dependencies properly support order book implementation.The additions of
rkyvandstatic_assertionsare well-utilized inorder_book.rsfor alignment guarantees and compile-time size/alignment checks.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
31-31: LGTM! New field enables diff-based commit path.The
request_diffflag properly controls whether to useCommitType::StandaloneDifforCommitType::Standalone, enabling the optimization for large accounts with small changes.programs/magicblock/src/magicblock_processor.rs (2)
44-59: LGTM! Existing instructions properly default to non-diff path.Both
ScheduleCommitandScheduleCommitAndUndelegatecorrectly setrequest_diff: falseto maintain existing behavior.
85-92: LGTM! New instruction variant correctly enables diff-based commit with undelegation.The
ScheduleCommitDiffAndUndelegateinstruction properly sets bothrequest_undelegation: trueandrequest_diff: true, mirroringScheduleCommitAndUndelegatebehavior while enabling the diff optimization.test-integration/programs/schedulecommit/src/order_book.rs (1)
48-75: LGTM! Mutable aliasing UB resolved with aligned copy.The implementation now safely copies the input buffer into
rkyv::AlignedVecbefore creating mutable references, avoiding the undefined behavior from the previous approach. The comment clearly explains the rationale.magicblock-committor-service/src/tasks/args_task.rs (2)
171-172: LGTM! CommitDiff correctly returnsErr(self)in optimize().Returning
Err(self)forCommitDiffis correct since the diff is already optimized by design—it sends only the differences rather than full account data. Converting toBufferTaskwould be counterproductive.Per previous discussion, this is the correct semantic since CommitDiff is already using the most efficient data transfer strategy.
196-203: LGTM! Compute units appropriately set for CommitDiff.The 65,000 compute units for
CommitDiff(vs 70,000 forCommit) reflects the reduced work from transmitting only the diff. This aligns with the test observation showing lower CU usage.Based on learnings: these are compute units per task, and transactions may comprise multiple tasks.
test-integration/programs/schedulecommit/src/lib.rs (3)
203-203: LGTM! Type change tou64is appropriate.Changing
MainAccount::SIZEfromusizetou64aligns with thesizeparameter type inallocate_account_and_assign_owner(line 255) and Solana's account size conventions.
398-414: LGTM! Log message and signer check are correct.Line 402 now uses the accurate log message "Update orderbook", and line 407 properly validates that the payer is a signer before allowing the update.
419-439: LGTM! Handler correctly uses diff-based commit with undelegation.The implementation properly:
- Returns
NotEnoughAccountKeyswhen accounts are missing (line 426)- Validates payer is a signer (line 429)
- Invokes
commit_diff_and_undelegate_accountsfor the optimized diff path (line 431)
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
99f2e8f to
7eaaa13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/args_task.rs (1)
228-235: Critical:reset_commit_idsilently ignores CommitDiff tasks.When a
CommitDifftask is recycled, the current code silently returns without updating itscommit_id(line 230-232), leaving stale IDs that will cause:
- Desynchronization with actual commit state
- Failed submissions with wrong nonce
- Data integrity issues
A past review correctly identified this issue.
Apply this fix to handle both
CommitandCommitDiff:fn reset_commit_id(&mut self, commit_id: u64) { - // TODO (snawaz): handle CommitDiff as well? what is it about? - let ArgsTaskType::Commit(commit_task) = &mut self.task_type else { - return; - }; - - commit_task.commit_id = commit_id; + match &mut self.task_type { + ArgsTaskType::Commit(task) | ArgsTaskType::CommitDiff(task) => { + task.commit_id = commit_id; + } + _ => { + // Other task types don't have commit_id + } + } }
♻️ Duplicate comments (7)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
389-446: CRITICAL: try_from_args cannot construct StandaloneDiff.The
try_from_argsmethod only handlesCommitTypeArgs::StandaloneandCommitTypeArgs::WithBaseActions, but does not handleCommitTypeArgs::StandaloneDiff. This means:
- Code paths using
try_from_args(e.g.,process_schedule_base_intent) cannot constructCommitType::StandaloneDiff- Only
process_schedule_commitcan create it programmatically- Any attempt to deserialize
CommitTypeArgs::StandaloneDifffrom instruction data will failThis creates a critical inconsistency where the
StandaloneDiffvariant cannot be fully utilized across all code paths.The method needs a new match arm similar to:
pub fn try_from_args( args: CommitTypeArgs, context: &ConstructionContext<'_, '_>, ) -> Result<CommitType, InstructionError> { match args { CommitTypeArgs::Standalone(accounts) => { // ... existing code ... Ok(CommitType::Standalone(committed_accounts)) } + CommitTypeArgs::StandaloneDiff(accounts) => { + let committed_accounts_ref = Self::extract_commit_accounts( + &accounts, + context.transaction_context, + )?; + Self::validate_accounts(&committed_accounts_ref, context)?; + let committed_accounts = committed_accounts_ref + .into_iter() + .map(|el| { + let mut committed_account: CommittedAccount = el.into(); + committed_account.account.owner = context + .parent_program_id + .unwrap_or(committed_account.account.owner); + committed_account + }) + .collect(); + Ok(CommitType::StandaloneDiff(committed_accounts)) + } CommitTypeArgs::WithBaseActions { ... } => { // ... existing code ... } } }Based on past review comment.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
224-228: Add observability logging for commit type selection.The conditional logic correctly selects the commit type, but lacks logging to indicate which path was taken. This makes debugging and auditing more difficult.
Based on past review comment, consider adding:
let commit_action = if opts.request_diff { + ic_msg!( + invoke_context, + "ScheduleCommit: using StandaloneDiff for {} accounts", + committed_accounts.len() + ); CommitType::StandaloneDiff(committed_accounts) } else { + ic_msg!( + invoke_context, + "ScheduleCommit: using Standalone for {} accounts", + committed_accounts.len() + ); CommitType::Standalone(committed_accounts) };magicblock-committor-service/src/tasks/args_task.rs (1)
74-122: Network I/O in instruction() confirmed as unresolved; improve fallback logging.The RPC-in-instruction issue is genuine and remains present despite the past review claiming it was addressed in commit 99f2e8f (which does not exist in the repository).
Confirmed concerns:
instruction() called multiple times per task: The method is invoked from
delivery_preparator.rs(line 377, mapped over cleanup_tasks),tasks/utils.rs(line 58, mapped over tasks), andtask_strategist.rs(line 217, in optimization loop). Each call triggers RPC I/O on the base-chain, multiplying network overhead and creating TOCTOU race conditions.ChainConfig hardcoded locally (line 76): ChainConfig is properly injected into the service/processor but ArgsTask hardcodes
ChainConfig::local()instead of receiving injected config. This remains unresolved.Fallback log incomplete (line 90): The message contains a typo ("commmit_id" with 3 m's) and is missing the
pubkeycontext for debugging. Improve clarity:- log::warn!("Fallback to commit_state and send full-bytes, as rpc failed to fetch the delegated-account from base chain, commmit_id: {} , error: {}", value.commit_id, e); + log::warn!( + "Fallback to commit_state for account {}: RPC fetch failed, commit_id={} - {}", + value.committed_account.pubkey, + value.commit_id, + e + );The architectural concerns about performing RPC I/O during instruction building should be addressed, and the config injection pattern needs to be established.
test-integration/programs/schedulecommit/src/lib.rs (3)
280-311: Header remains uninitialized after allocation—subsequent reads are undefined.After
allocate_account_and_assign_owner, the header fields (bids_len,asks_len) are still uninitialized. Any call toOrderBook::newon this account will read garbage.Apply this fix to zero the header immediately after allocation:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + data[..core::mem::size_of::<OrderBookHeader>()].fill(0); Ok(())
329-334: Error message references wrong key.The PDA assertion message incorrectly shows
payer.keyinstead ofbook_manager.key, making debugging harder.assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
402-402: Inconsistent log message casing.Message says "Update orderbook" but other logs use "OrderBook" with capital letters and space for consistency.
- msg!("Update orderbook"); + msg!("Update order book");test-integration/Cargo.toml (1)
40-40: Local path dependencies will break CI/CD builds.These local path dependencies point to repositories outside the current codebase (
../../ephemeral-rollups-sdkand../../delegation-program) that are not cloned by the CI workflow. This will cause build failures for all contributors and in continuous integration.Choose one of the following solutions:
- Revert to git-based dependencies (recommended for now):
-ephemeral-rollups-sdk = { path = "../../ephemeral-rollups-sdk/rust/sdk"} +ephemeral-rollups-sdk = { git = "https://github.com/magicblock-labs/ephemeral-rollups-sdk.git", rev = "..." }
Update CI to clone required repositories before build steps in
.github/workflows/ci-test-integration.ymlDocument the local setup in README with explicit clone instructions if local paths are intended for development only
Based on learnings: This appears to be a transition to local development setup but lacks the corresponding CI infrastructure updates.
Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
magicblock-committor-service/src/tasks/args_task.rs(6 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(7 hunks)programs/magicblock/src/magicblock_processor.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(2 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(9 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/args_task.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/programs/schedulecommit/src/order_book.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/args_task.rs (4)
magicblock-committor-service/src/tasks/mod.rs (2)
instruction(66-66)instruction(249-255)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/config.rs (1)
local(29-35)magicblock-accounts/src/scheduled_commits_processor.rs (2)
new(66-90)new(420-436)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)
programs/magicblock/src/magicblock_processor.rs (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
process_schedule_commit(34-258)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(153-156)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(544-549)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_test
🔇 Additional comments (14)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
29-32: Field addition looks correct.The new
request_difffield is properly scoped aspub(crate)and theDefaulttrait will initialize it tofalse, maintaining backward compatibility.programs/magicblock/src/magicblock_processor.rs (2)
44-51: ScheduleCommit handler correctly configured.The handler properly sets
request_diff: falseto maintain the existing full-commit behavior for standard commits.
85-92: New instruction handler correctly implemented.The
ScheduleCommitDiffAndUndelegatehandler properly enables both undelegation and diff-based commits by setting both flags totrue. The implementation follows the established pattern from the other handlers.programs/magicblock/src/magic_scheduled_base_intent.rs (4)
104-106: Delegation method correctly implemented.The
is_commit_diff()method appropriately delegates to the underlyingbase_intent, following the established pattern for other query methods in this struct.
155-163: Commit diff detection logic is correct.The method correctly returns
falseforBaseActionsand properly delegates to the commit-related variants to determine if differential commits are enabled.
317-327: StandaloneDiff variant added correctly.The new variant follows the same structure as
Standalone, holdingVec<CommittedAccount>. Note that full account data is stored here; the actual diff computation likely occurs downstream during commit processing.
448-484: Accessor methods properly handle StandaloneDiff.All match expressions are now exhaustive and correctly handle the new
StandaloneDiffvariant. Theis_commit_diff()method correctly returnstrueonly forStandaloneDiff, and the accessor methods treat it consistently withStandalone.magicblock-committor-service/src/tasks/args_task.rs (4)
1-24: LGTM: Imports support the CommitDiff feature.All new imports are necessary for the RPC client setup, diff computation, and configuration.
30-30: LGTM: CommitDiff variant added.Reusing
CommitTaskfor theCommitDiffvariant is appropriate since they share the same data requirements.
171-172: LGTM: ReturningErr(self)for CommitDiff is correct.The past review discussion thoroughly analyzed this and concluded that
CommitDiffshould returnErr(self)(no optimization) because:
CommitDiffis already optimized by design—it transmits only the diff (e.g., 286 bytes) vs. full account data (10,240 bytes per your test)- Converting
CommitDifftoBufferTaskwould be counterproductive and defeat its purposeErr(self)correctly signals "no further optimization possible"Suggestion: The TODO on line 171 can be resolved and removed based on the past review's analysis.
199-199: LGTM: Compute units appropriately reduced for CommitDiff.The 65,000 compute unit allocation (5k less than
Commit's 70k) aligns with your test observations and reflects the reduced data processing for diff-based commits.test-integration/programs/schedulecommit/src/lib.rs (1)
419-439: LGTM—signer validation and error handling are now correct.The function now properly returns
NotEnoughAccountKeyswhen the accounts array doesn't match, and validates thatpayeris a signer before proceeding with the CPI call.test-integration/programs/schedulecommit/Cargo.toml (1)
11-12: LGTM—dependencies appropriately support zero-copy OrderBook.The
rkyvcrate providesAlignedVecfor safe buffer allocation, andstatic_assertionsenables compile-time layout verification. Both are correctly utilized inorder_book.rs.test-integration/Cargo.toml (1)
72-72: LGTM—workspace dependencies properly declared.The
rkyvandstatic_assertionscrates are correctly added at the workspace level, enabling their use across integration test crates for zero-copy structures and compile-time assertions.Also applies to: 91-91
GabrielePicco
left a comment
GabrielePicco
left a comment
There was a problem hiding this comment.
Thanks @snawaz. LGTM!
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Looks like generally this is going in the right direction, but wanted to leave some comments right away:
- We need to have 2 separate type
CommitTask&CommitWithDiffTask.CommitTaskcould be renamed into something to highlight that it isn't diff - Decision of whether to commit with diff or not shall be done in
TaskBuilder, it has all necessary knowledge to choose betweenCommitTask&CommitWithDiffTask. CommitTask & CommitWithDiffTask contain already complete information to be executed on chain. This will allow to get rid from clumsy Options and boolean flags in CommitTask - Task is representation of some instruction for some program, it can't know anything about Rpc or some network configs we use, so the logic of fetching account data for computing the diff shall be placed into TaskInfoFetcher
|
|
||
| use crate::{config::ChainConfig, ComputeBudgetConfig}; | ||
|
|
||
| let chain_config = |
There was a problem hiding this comment.
Task shouldn't be aware of the RpcClient, not be able to fetch anything from chain. Task is sort of representation of some chain instruction we want to execute.
The whole fetching of necessary info shall be moved into TaskInfoFetcher and assembly of Task shall be moved into TaskBuilder
| pub commit_id: u64, | ||
| pub allow_undelegation: bool, | ||
| pub committed_account: CommittedAccount, | ||
| base_account: Option<Account>, |
There was a problem hiding this comment.
CommitTask maybe ambiguos name now since when we were implementing this, we didn't think about another commit option via diffs. Since task is representation of instruction that is performed on chain we need to add new type of task for diffs. CommitWithDiffTask.
Please also rename CommitTask -> CommitDataTask or something like that to highlight that it commit of the whole account state and not the diff
There was a problem hiding this comment.
This will allow to remove clumsy Option<Account> that doesn't really make sense in the context of Commit diff task. If it is commit diif, then account against which we compute diff shall be present, if it is not present(for some weird reason, likely this should be an error) we shall choose CommitState instead.
100% agree. The logic for fetching base accounts inside the task isn’t a final solution ... it’s a temporary workaround. In the subsequent PRs, this logic will be moved out and put somewhere else... probably in With that, there might be some small refactoring too. I might make it a different task (like you said, So yes, that’s already in plan. It’ll just happen in the next set of PRs. Apart from that, is there anything you want me to change? Something trivial that can be done in this PR itself? |
|
@snawaz Noted, let's then make this PR ready for review and I will go over it again. The thing is that at the moment its not ready to be merged into master as its very raw. I'd recommend: add buffers, then modify We can review buffer PR & merge it separatly into this one, after that I will swiftly rereview this one and we will merge it |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/test-committor-service/tests/common.rs (1)
150-168: Consider reusing AccountFetcher instance across test tasks.The helper creates a new
AccountFetcherfor eachCommitTask. SinceAccountFetcher::new()performs configuration parsing and RPC client creation, consider creating it once and passing it as a parameter for efficiency.-pub async fn create_commit_task(data: &[u8]) -> CommitTask { +pub async fn create_commit_task(data: &[u8], account_fetcher: &AccountFetcher) -> CommitTask { static COMMIT_ID: AtomicU64 = AtomicU64::new(0); CommitTask::new( COMMIT_ID.fetch_add(1, Ordering::Relaxed), false, CommittedAccount { pubkey: Pubkey::new_unique(), account: Account { lamports: 1000, data: data.to_vec(), owner: dlp::id(), executable: false, rent_epoch: 0, }, }, - AccountFetcher::new(), + account_fetcher.clone(), // or pass by reference if AccountFetcher supports it ) .await }However, this optimization is optional for test code and can be deferred.
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/mod.rs (1)
121-123: Document the COMMIT_STATE_SIZE_THRESHOLD rationale.The 200-byte threshold determines when to use CommitDiff vs CommitState, but the reasoning is not documented.
Add a doc comment explaining:
- Why 200 bytes was chosen
- Trade-offs (RPC overhead vs instruction size)
- Whether this should be configurable
+ // Accounts larger than 200 bytes use CommitDiff to reduce on-chain instruction size. + // Below 200 bytes, the RPC overhead (network latency + serialization) of fetching + // base-chain state exceeds the benefit of sending a diff instead of full state. + // TODO: Make this threshold configurable based on measured RPC latency. const COMMIT_STATE_SIZE_THRESHOLD: usize = 200;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
magicblock-committor-service/Cargo.toml(1 hunks)magicblock-committor-service/src/tasks/account_fetcher.rs(1 hunks)magicblock-committor-service/src/tasks/mod.rs(8 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)test-integration/test-committor-service/tests/common.rs(3 hunks)test-integration/test-committor-service/tests/test_delivery_preparator.rs(8 hunks)test-integration/test-committor-service/tests/test_transaction_preparator.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/test_delivery_preparator.rsmagicblock-committor-service/src/tasks/task_builder.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (6)
magicblock-committor-service/src/tasks/account_fetcher.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)magicblock-committor-service/src/config.rs (1)
mainnet(21-27)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (3)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(16-40)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)test-integration/test-committor-service/tests/common.rs (2)
new(52-55)create_commit_task(150-168)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(16-40)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)
test-integration/test-committor-service/tests/test_transaction_preparator.rs (4)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(16-40)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)magicblock-committor-service/src/tasks/args_task.rs (1)
new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
magicblock-committor-service/src/tasks/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-85)new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (3)
instruction(65-86)new(37-45)new_preparation_required(30-35)magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(16-40)
test-integration/test-committor-service/tests/common.rs (2)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(16-40)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)
🔇 Additional comments (7)
magicblock-committor-service/Cargo.toml (1)
32-32: LGTM - dependency addition is appropriate.The
magicblock-configworkspace dependency is correctly added to supportAccountFetcherconfiguration parsing.test-integration/test-committor-service/tests/test_delivery_preparator.rs (2)
19-21: Document the multi-threaded runtime requirement and track removal.The TODO correctly identifies that
CommitTask::new()uses a blockingRpcClient, forcing multi-threaded runtime. However, this is a temporary workaround.Per the PR discussion and past review comments, the RPC fetching logic should be moved out of tasks into
TaskInfoFetcher. Tasks should not be network-aware.Questions:
- Is there a tracking issue for moving RPC logic to
TaskInfoFetcher?- What's the timeline for removing this workaround?
Based on PR objectives and past review comments.
91-103: Concurrent task construction is well-implemented.Using
join_allto construct commit tasks concurrently is a good optimization that reduces sequential bottlenecks when multiple tasks need account fetching.test-integration/test-committor-service/tests/test_transaction_preparator.rs (1)
39-47: Tests properly adapted to new async API.The tests correctly await
CommitTask::new()and provideAccountFetcher. The pattern is consistent across all test cases.Note: The repeated
AccountFetcher::new()calls are inefficient but acceptable for test code.magicblock-committor-service/src/tasks/mod.rs (3)
162-211: Commit instruction creation logic is sound.The branching between
create_commit_state_ixandcreate_commit_diff_ixbased on the presence ofbase_accountis correct. The diff computation usingcompute_diffis properly integrated.
65-66: Track the involved_accounts performance optimization.The TODO correctly identifies that
involved_accounts()builds the fullInstruction(potentially triggering account fetches and diff computation) just to extract account pubkeys. This is wasteful.Consider creating a lightweight method that returns only pubkeys without constructing the full instruction, or caching the result.
Do you want me to create a tracking issue for this optimization?
473-497: Test properly updated to async constructor.The serialization test correctly uses the async
CommitTask::new()and the test attribute is properly marked with#[tokio::test].
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-committor-service/src/tasks/account_fetcher.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(125-149)magicblock-committor-service/src/config.rs (2)
local(29-35)mainnet(21-27)
…fficiently commit changes in delegated accounts
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
8ab8c00 to
4ea5450
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
28-42: Consider consistent formatting for constructor calls.The two constructor calls use different formatting styles: line 30 uses a single-line format while lines 32-35 spread the call across multiple lines. Both calls have similar argument counts and length, so consider using consistent formatting for improved readability.
Apply this diff for consistency:
let ctx = if std::env::var("FIXED_KP").is_ok() { ScheduleCommitTestContext::try_new(2, b"magic_schedule_commit") } else { - ScheduleCommitTestContext::try_new_random_keys( - 2, - b"magic_schedule_commit", - ) + ScheduleCommitTestContext::try_new_random_keys(2, b"magic_schedule_commit") }test-integration/test-committor-service/tests/test_delivery_preparator.rs (1)
161-176: Match the runtime flavor with other commit-task tests
test_lookup_tablesstill runs on the current-thread runtime, but it callscreate_commit_task(..).await, which drivesCommitTask::new(). That constructor still performs blockingRpcClientcalls, so on a single-thread runtime it can monopolize the only worker and starve the futures spawned insideprepare_for_delivery, leading to deadlocks/timeouts when the RPC call stalls. Please switch this test to the same multi-thread configuration used above until the constructor stops blocking.-#[tokio::test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)]magicblock-committor-service/src/tasks/mod.rs (1)
420-439: Fix: Test must be async to call async CommitTask::new().
CommitTask::new()is async (line 125), but this test function is not marked asasyncand the call lacks.await. This will cause a compilation error.Apply this diff:
- #[test] - fn test_args_task_instruction_serialization() { + #[tokio::test] + async fn test_args_task_instruction_serialization() { let validator = Pubkey::new_unique(); // Test Commit variant - let commit_task: ArgsTask = ArgsTaskType::Commit(CommitTask::new( + let commit_task: ArgsTask = ArgsTaskType::Commit(CommitTask::new( 123, true, CommittedAccount { pubkey: Pubkey::new_unique(), account: Account { lamports: 1000, data: vec![1, 2, 3], owner: Pubkey::new_unique(), executable: false, rent_epoch: 0, }, }, + AccountFetcher::new(), - )) + ).await) .into();
♻️ Duplicate comments (17)
test-integration/programs/schedulecommit/src/utils/mod.rs (1)
71-71: Unchecked cast still present.The unchecked
as usizecast was previously flagged for defensive coding. While safe in practice (Solana runs 64-bit), a checked conversion would be more robust.test-integration/schedulecommit/client/src/verify.rs (1)
16-22: Deduplicate the log-verification helperThis helper repeats
fetch_and_verify_commit_result_from_logsverbatim. Please fold both call sites into a single generic that accepts the result type so we keep the verification logic in one place.To illustrate the idea:
-pub fn fetch_and_verify_commit_result_from_logs( - ctx: &ScheduleCommitTestContext, - sig: Signature, -) -> ScheduledCommitResult<MainAccount> { - let res = ctx.fetch_schedule_commit_result(sig).unwrap(); - res.confirm_commit_transactions_on_chain(ctx).unwrap(); - res -} - -pub fn fetch_and_verify_order_book_commit_result_from_logs( - ctx: &ScheduleCommitTestContext, - sig: Signature, -) -> ScheduledCommitResult<OrderBookOwned> { - let res = ctx.fetch_schedule_commit_result(sig).unwrap(); - res.confirm_commit_transactions_on_chain(ctx).unwrap(); - res -} +pub fn fetch_and_verify_commit_result_from_logs<T>( + ctx: &ScheduleCommitTestContext, + sig: Signature, +) -> ScheduledCommitResult<T> +where + T: borsh::BorshDeserialize, +{ + let res = ctx.fetch_schedule_commit_result(sig).unwrap(); + res.confirm_commit_transactions_on_chain(ctx).unwrap(); + res +}Callers can then select the concrete type with turbofish (e.g.
::<MainAccount>/::<OrderBookOwned>).magicblock-committor-service/src/tasks/task_builder.rs (1)
91-103: Reuse a singleAccountFetcherwhen building commit tasks
AccountFetcher::new()spins up an RPC client and parses chain config. Calling it once per account means 20 accounts spawn 20 clients in parallel, multiply hitting config parsing and the RPC endpoint. Please hoist a single fetcher outside the loop (make itCloneor wrap it in anArc) and reuse it inside the async blocks so we only build the client once.Example fix sketch:
- let tasks = join_all(accounts - .iter() - .map(|account| async { - let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); - let task = ArgsTaskType::Commit(CommitTask::new( - commit_id, - allow_undelegation, - account.clone(), - AccountFetcher::new(), - ).await); - - Box::new(ArgsTask::new(task)) as Box<dyn BaseTask> - })).await; + let account_fetcher = AccountFetcher::new(); + let tasks = join_all( + accounts.iter().map(|account| { + let account = account.clone(); + let account_fetcher = account_fetcher.clone(); + let commit_id = *commit_ids + .get(&account.pubkey) + .expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!"); + async move { + let task = ArgsTaskType::Commit( + CommitTask::new( + commit_id, + allow_undelegation, + account, + account_fetcher, + ) + .await, + ); + Box::new(ArgsTask::new(task)) as Box<dyn BaseTask> + } + }), + ) + .await;(If
AccountFetcheris not yetClone, wrap it in anArcand adjustCommitTask::newto accept that.)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
160-273: Swap the new println! calls for the existing log macros.This module already relies on
debug!/info!, and we previously asked to keep stdout quiet. Please route the diagnostics in the helper (line 160) and the RNG/reporting block (lines 253‑273) through the logging macros or gate them behind a verbosity flag.test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (2)
26-32: Use log macros instead of println! in shared test helpers.These helpers execute in every scenario; we already asked to avoid unconditional stdout noise. Please swap the new
println!calls fordebug!/info!(or guard them behind a verbosity flag) so the suite stays quiet by default.
45-47: Drop the unused trait bounds onT.The body never inspects or formats
T, so requiringDebug + BorshDeserialize + PartialEq + Eqonly burdens new call sites—the same point raised earlier. Please remove these bounds unless they become necessary.test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
193-206: Remove the commented-out grow_order_book block.This TODO block was already flagged—please track the follow-up elsewhere and keep the helper free of dead/commented code.
208-210: Renderuser_seederrors in readable text.Printing the byte slice with
{:?}yields numeric output; earlier feedback asked to display the seed as UTF‑8 (e.g.,String::from_utf8_lossy(&self.user_seed)) so the message is human-friendly.test-integration/programs/schedulecommit/src/api.rs (1)
125-142: Make the seed handling explicit indelegate_account_cpi_instruction.Falling through to the order-book variant for any non-
"magic_schedule_commit"seed will quietly build an instruction the program rejects. Please match the known seeds and return an error (or panic) for anything else so misconfiguration fails fast.magicblock-committor-service/src/tasks/mod.rs (2)
112-118: Refactor: Split CommitTask into distinct task types and remove Option fields.The
Option<Account>andforce_commit_stateflag mix path-selection logic into the task. As previously recommended, introduce separate task types (CommitStateTaskandCommitDiffTask) to make the commit strategy explicit at construction time. This removes the need for optional fields and runtime branching, making each task's purpose clear.Move the decision logic (size threshold check, account fetching) into
TaskBuilder, which has the necessary context to choose the appropriate task type upfront.Based on past review comments.
121-123: Consider documenting the rationale for the 200-byte threshold.The comment explains what the threshold does but not why 200 bytes was chosen. Consider adding a brief note about the trade-offs (RPC overhead vs instruction data size) and whether this should be configurable in the future.
test-integration/programs/schedulecommit/src/lib.rs (3)
278-309: Initialize OrderBook header after allocation.After
allocate_account_and_assign_owner, the account data contains uninitialized bytes. TheOrderBookHeaderfields (bids_len,asks_len) must be explicitly zeroed before any subsequent code attempts to read them.Apply this diff:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + let header = OrderBookHeader { bids_len: 0, asks_len: 0 }; + data[0..8].copy_from_slice(&[ + header.bids_len.to_le_bytes().as_ref(), + header.asks_len.to_le_bytes().as_ref(), + ].concat()); Ok(()) }Based on past review comments.
327-332: Fix error message: use book_manager.key instead of payer.key.The PDA assertion error message incorrectly references
payer.keyinstead ofbook_manager.key, which will confuse debugging.Apply this diff:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;Based on past review comments.
334-350: Guard against overflow in size calculation.The expression
order_book.data_len() + additional_space as usizecan overflow on BPF in release builds, potentially causing the realloc to silently truncate the account.Apply this diff:
- let new_size = order_book.data_len() + additional_space as usize; + let new_size = order_book + .data_len() + .checked_add(additional_space as usize) + .ok_or(ProgramError::InvalidArgument)?;Based on past review comments.
test-integration/programs/schedulecommit/src/order_book.rs (3)
127-130: Propagate capacity exhaustion errors instead of silently ignoring them.
update_fromignores theOptionreturned byadd_bidsandadd_asks. If capacity is exhausted, updates fail silently, potentially causing partial application (e.g., bids succeed but asks are dropped).Apply this diff to propagate failures:
- pub fn update_from(&mut self, updates: BookUpdate) { - self.add_bids(&updates.bids); - self.add_asks(&updates.asks); + pub fn update_from(&mut self, updates: BookUpdate) -> Result<(), &'static str> { + self.add_bids(&updates.bids).ok_or("insufficient capacity for bids")?; + self.add_asks(&updates.asks).ok_or("insufficient capacity for asks")?; + Ok(()) }Based on past review comments.
171-184: Alignment is checked, but consider byte-level accessors for maximum safety.The alignment assertions in
OrderBook::new()(lines 108-116) mitigate the risk of unaligned references. However, if you want to be extra cautious (as suggested in past reviews), consider storing levels as raw bytes and providing accessors that read/write viau64::from_le_bytes/to_le_bytesto avoid typed references entirely.The current approach with runtime alignment checks is reasonable for test code.
Based on past review comments.
211-215: Prevent integer overflow in capacity calculation.The expression
bids_len + asks_lenis computed using u32 arithmetic and can overflow before being cast tousize. For example, ifbids_len = u32::MAXandasks_len = 1, the sum wraps to 0, causingchecked_subto incorrectly succeed.Apply this diff:
fn remaining_capacity(&self) -> usize { + let total_used = (self.header.bids_len as usize) + .checked_add(self.header.asks_len as usize) + .expect("bids_len + asks_len overflows usize"); self.capacity - .checked_sub((self.header.bids_len + self.header.asks_len) as usize) + .checked_sub(total_used) .expect("remaining_capacity must exist") }Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (29)
Cargo.toml(1 hunks)magicblock-committor-service/Cargo.toml(1 hunks)magicblock-committor-service/src/tasks/account_fetcher.rs(1 hunks)magicblock-committor-service/src/tasks/args_task.rs(3 hunks)magicblock-committor-service/src/tasks/mod.rs(8 hunks)magicblock-committor-service/src/tasks/task_builder.rs(3 hunks)magicblock-committor-service/src/tasks/task_strategist.rs(3 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(1 hunks)test-integration/Cargo.toml(4 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(8 hunks)test-integration/programs/schedulecommit/src/order_book.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(3 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(6 hunks)test-integration/schedulecommit/client/src/verify.rs(2 hunks)test-integration/schedulecommit/test-scenarios/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(4 hunks)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs(1 hunks)test-integration/schedulecommit/test-security/tests/01_invocations.rs(1 hunks)test-integration/test-committor-service/tests/common.rs(3 hunks)test-integration/test-committor-service/tests/test_delivery_preparator.rs(8 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(10 hunks)test-integration/test-committor-service/tests/test_transaction_preparator.rs(4 hunks)test-integration/test-committor-service/tests/utils/transactions.rs(1 hunks)test-integration/test-ledger-restore/tests/08_commit_update.rs(2 hunks)test-integration/test-tools/src/integration_test_context.rs(2 hunks)test-integration/test-tools/src/scheduled_commits.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-tools/src/integration_test_context.rsmagicblock-committor-service/src/tasks/args_task.rstest-integration/test-ledger-restore/tests/08_commit_update.rstest-integration/test-tools/src/scheduled_commits.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/schedulecommit/test-security/tests/01_invocations.rstest-integration/programs/schedulecommit/src/utils/mod.rstest-integration/schedulecommit/client/src/verify.rstest-integration/test-committor-service/tests/test_delivery_preparator.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rsmagicblock-committor-service/src/tasks/task_builder.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/args_task.rsprograms/magicblock/src/magic_scheduled_base_intent.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/order_book.rsmagicblock-committor-service/src/tasks/account_fetcher.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
programs/magicblock/src/magic_scheduled_base_intent.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/schedulecommit/client/src/schedule_commit_context.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
test-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-committor-service/src/tasks/account_fetcher.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/programs/schedulecommit/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T16:53:29.820Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:134-0
Timestamp: 2025-10-26T16:53:29.820Z
Learning: In magicblock-chainlink/src/remote_account_provider/mod.rs, the `Endpoint::separate_pubsub_url_and_api_key()` method uses `split_once("?api-key=")` because the api-key parameter is always the only query parameter right after `?`. No additional query parameter parsing is needed for this use case.
Applied to files:
magicblock-committor-service/src/tasks/account_fetcher.rs
🧬 Code graph analysis (18)
test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/args_task.rs (1)
new(36-41)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(125-156)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
test-integration/test-committor-service/tests/common.rs (2)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-156)
test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
test-integration/test-ledger-restore/src/lib.rs (1)
setup_validator_with_local_remote(97-119)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
try_new(72-74)try_new_random_keys(66-71)
test-integration/schedulecommit/client/src/verify.rs (2)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)
test-integration/test-committor-service/tests/test_delivery_preparator.rs (2)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
join_all(467-476)new(49-59)test-integration/test-committor-service/tests/common.rs (3)
new(52-55)generate_random_bytes(142-147)create_commit_task(150-168)
test-integration/test-committor-service/tests/test_transaction_preparator.rs (3)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-156)magicblock-committor-service/src/tasks/buffer_task.rs (2)
new(37-45)new_preparation_required(30-35)
magicblock-committor-service/src/tasks/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
instruction(45-85)new(36-41)magicblock-committor-service/src/tasks/buffer_task.rs (2)
instruction(65-86)new(37-45)magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)
test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (3)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
ctx(148-149)test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
ctx(90-91)test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
try_new(72-74)ncommittees(102-121)try_new_random_keys(66-71)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)magicblock-committor-service/src/tasks/mod.rs (1)
new(125-156)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (3)
test-integration/programs/schedulecommit/src/api.rs (2)
schedule_commit_diff_instruction_for_order_book(196-215)update_order_book_instruction(178-194)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-35)test-integration/schedulecommit/client/src/verify.rs (1)
fetch_and_verify_order_book_commit_result_from_logs(16-23)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (3)
test-integration/programs/schedulecommit/src/api.rs (3)
init_order_book_instruction(36-54)init_payer_escrow(77-98)init_account_instruction(16-34)test-integration/test-tools/src/integration_test_context.rs (1)
try_new(113-115)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/programs/schedulecommit/src/api.rs (1)
test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
magicblock-committor-service/src/tasks/account_fetcher.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(125-156)magicblock-committor-service/src/config.rs (2)
local(29-35)mainnet(21-27)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
magicblock-validator/src/main.rs (1)
init_logger(13-50)
🪛 GitHub Actions: Run CI - Format
magicblock-committor-service/src/tasks/mod.rs
[error] 1-1: Rustfmt diff detected in mod.rs. Formatting changes are required. Run 'cargo fmt' to fix code style issues.
[error] 23-23: Rustfmt diff detected in mod.rs (line 23). Formatting changes are required. Run 'cargo fmt' to fix code style issues.
magicblock-committor-service/src/tasks/task_builder.rs
[error] 9-9: Rustfmt diff detected in task_builder.rs (line 9). Formatting changes are required. Run 'cargo fmt' to fix code style issues.
[error] 19-19: Rustfmt diff detected in task_builder.rs (line 19). Formatting changes are required. Run 'cargo fmt' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (12)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
53-53: Good change aligning with Solana's account size type.Changing the
sizefield fromusizetou64correctly matches Solana's native account size type, eliminating the need for conversions at the call site.
75-79: Helpful debugging addition.The logging of required lamports and payer balance aids troubleshooting of allocation and transfer issues in tests.
90-90: Clean improvement removing unnecessary conversion.Passing
sizedirectly tosystem_instruction::allocate(which expectsu64) eliminates the previoustry_into().unwrap()conversion, making the code cleaner and more straightforward.test-integration/test-committor-service/tests/utils/transactions.rs (1)
123-133: Implementation is correct and pragmatic.The special-case logic correctly handles the dual commit paths introduced by
CommitDiff. The inline comment clearly explains why "CommitState" matches both instruction types, avoiding the need to update numerous existing tests.Note the intentional asymmetry: searching for "CommitState" matches both commit types, while searching for "CommitDiff" matches only
CommitDiff. This allows tests that explicitly check for the diff path to remain specific.test-integration/schedulecommit/test-scenarios/Cargo.toml (1)
19-20: LGTM! Dev-dependency additions align with new test requirements.The addition of
randandborshas dev-dependencies appropriately supports the new OrderBook integration tests mentioned in the PR objectives, enabling random data generation and serialization testing.test-integration/test-ledger-restore/tests/08_commit_update.rs (1)
55-55: LGTM! Proper handling of expanded return tuple.The destructuring correctly captures the additional
TempDirreturn value with an underscore prefix, following Rust conventions for intentionally unused bindings.Also applies to: 170-170
test-integration/test-tools/src/integration_test_context.rs (2)
157-159: LGTM! Error message now properly formats the label.The panic message correctly interpolates the
labelvariable, resolving the previous issue where the placeholder was displayed literally.
168-174: LGTM! Transaction version logic is now documented.The inline comment clearly explains why the chain cluster requires explicit v0 transaction support while the ephemeral cluster uses default handling.
test-integration/Cargo.toml (1)
40-40: LGTM! Dependency updates appropriately support new features.
- The
ephemeral-rollups-sdkuses a git reference (not local path), addressing previous CI concerns.- The
magicblock-delegation-programrev bump toe8d03936aligns with the CommitDiff feature additions.- New dependencies
rkyvandstatic_assertionssupport zero-copy serialization and compile-time checks for the OrderBook integration.Also applies to: 60-60, 74-74, 93-93
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
312-325: LGTM! Clean abstraction for committed account representation.The
CommittedAccountstruct and itsFromimplementation properly encapsulate committed account state with correct ownership semantics (borrowing fromRefCell<AccountSharedData>and converting to ownedAccount).test-integration/test-tools/src/scheduled_commits.rs (1)
182-235: LGTM! Two-level log fetching is now clearly structured.The renaming to
ephem_logs_l1andephem_logs_l2clarifies the multi-stage log retrieval flow. The use of*siginstead ofsig.clone()correctly handles theCopytype, and the debug output appropriately aids test debugging.magicblock-committor-service/Cargo.toml (1)
32-32: LGTM! Dependency addition supports new AccountFetcher module.The
magicblock-configdependency is required by the newAccountFetcherto access configuration parsing for RPC URI resolution.
| pub fn new() -> Self { | ||
| use crate::{config::ChainConfig, ComputeBudgetConfig}; | ||
|
|
||
| #[cfg(feature = "dev-context-only-utils")] | ||
| let chain_config = | ||
| ChainConfig::local(ComputeBudgetConfig::new(1_000_000)); | ||
|
|
||
| //#[cfg(not(feature = "dev-context-only-utils"))] | ||
| let chain_config = ChainConfig { | ||
| rpc_uri: magicblock_config::MagicBlockConfig::parse_config() | ||
| .config | ||
| .accounts | ||
| .remote | ||
| .url | ||
| .as_ref() | ||
| .map(|url| url.to_string()) | ||
| .unwrap_or_else(|| { | ||
| log::error!( | ||
| "Remote URL not configured, falling back to mainnet" | ||
| ); | ||
| "https://api.mainnet-beta.solana.com".to_string() | ||
| }), | ||
| ..ChainConfig::mainnet(ComputeBudgetConfig::new(1_000_000)) | ||
| }; | ||
|
|
||
| Self { | ||
| rpc_client: RpcClient::new_with_commitment( | ||
| chain_config.rpc_uri.to_string(), | ||
| CommitmentConfig { | ||
| commitment: chain_config.commitment, | ||
| }, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Feature flag logic is broken—dev-context config is shadowed.
The dev-context-only-utils feature check creates a ChainConfig at line 18, but line 22 unconditionally creates another chain_config binding that shadows it. This means the dev-context configuration is never actually used, even when the feature is enabled. The commented-out #[cfg(not(feature = "dev-context-only-utils"))] on line 21 suggests the production block should have been guarded.
Apply this diff to fix the feature flag logic:
#[cfg(feature = "dev-context-only-utils")]
let chain_config =
ChainConfig::local(ComputeBudgetConfig::new(1_000_000));
- //#[cfg(not(feature = "dev-context-only-utils"))]
+ #[cfg(not(feature = "dev-context-only-utils"))]
let chain_config = ChainConfig {
rpc_uri: magicblock_config::MagicBlockConfig::parse_config()
.config
.accounts
.remote
.url
.as_ref()
.map(|url| url.to_string())
.unwrap_or_else(|| {
log::error!(
"Remote URL not configured, falling back to mainnet"
);
"https://api.mainnet-beta.solana.com".to_string()
}),
..ChainConfig::mainnet(ComputeBudgetConfig::new(1_000_000))
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn new() -> Self { | |
| use crate::{config::ChainConfig, ComputeBudgetConfig}; | |
| #[cfg(feature = "dev-context-only-utils")] | |
| let chain_config = | |
| ChainConfig::local(ComputeBudgetConfig::new(1_000_000)); | |
| //#[cfg(not(feature = "dev-context-only-utils"))] | |
| let chain_config = ChainConfig { | |
| rpc_uri: magicblock_config::MagicBlockConfig::parse_config() | |
| .config | |
| .accounts | |
| .remote | |
| .url | |
| .as_ref() | |
| .map(|url| url.to_string()) | |
| .unwrap_or_else(|| { | |
| log::error!( | |
| "Remote URL not configured, falling back to mainnet" | |
| ); | |
| "https://api.mainnet-beta.solana.com".to_string() | |
| }), | |
| ..ChainConfig::mainnet(ComputeBudgetConfig::new(1_000_000)) | |
| }; | |
| Self { | |
| rpc_client: RpcClient::new_with_commitment( | |
| chain_config.rpc_uri.to_string(), | |
| CommitmentConfig { | |
| commitment: chain_config.commitment, | |
| }, | |
| ), | |
| } | |
| } | |
| pub fn new() -> Self { | |
| use crate::{config::ChainConfig, ComputeBudgetConfig}; | |
| #[cfg(feature = "dev-context-only-utils")] | |
| let chain_config = | |
| ChainConfig::local(ComputeBudgetConfig::new(1_000_000)); | |
| #[cfg(not(feature = "dev-context-only-utils"))] | |
| let chain_config = ChainConfig { | |
| rpc_uri: magicblock_config::MagicBlockConfig::parse_config() | |
| .config | |
| .accounts | |
| .remote | |
| .url | |
| .as_ref() | |
| .map(|url| url.to_string()) | |
| .unwrap_or_else(|| { | |
| log::error!( | |
| "Remote URL not configured, falling back to mainnet" | |
| ); | |
| "https://api.mainnet-beta.solana.com".to_string() | |
| }), | |
| ..ChainConfig::mainnet(ComputeBudgetConfig::new(1_000_000)) | |
| }; | |
| Self { | |
| rpc_client: RpcClient::new_with_commitment( | |
| chain_config.rpc_uri.to_string(), | |
| CommitmentConfig { | |
| commitment: chain_config.commitment, | |
| }, | |
| ), | |
| } | |
| } |
🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/account_fetcher.rs around lines 14 to
47, the dev-context ChainConfig created under #[cfg(feature =
"dev-context-only-utils")] is being shadowed by an unconditional production
ChainConfig; make the production branch conditional so the dev config is
actually used when the feature is enabled. Replace the unconditional production
binding with a cfg-guarded one (e.g., #[cfg(not(feature =
"dev-context-only-utils"))] on the production block) or refactor to a single let
chain_config = if cfg!(feature = "dev-context-only-utils") { ... } else { ... }
so only one binding is created and the correct config is selected based on the
feature flag.
| pub async fn fetch_account( | ||
| &self, | ||
| pubkey: &Pubkey, | ||
| ) -> Result<Account, solana_rpc_client_api::client_error::Error> { | ||
| self.rpc_client.get_account(pubkey).await | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add observability for account fetch operations.
The fetch_account method lacks tracing, making it difficult to diagnose failures in production. Consider adding instrumentation to track fetch attempts, latency, and errors as suggested in the previous review.
Based on learnings
| // TODO (snawaz): rewrite it. | ||
| // currently it is slow as it discards heavy computations and memory allocations. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Track optimization: involved_accounts builds full instruction unnecessarily.
The TODO is valid. involved_accounts() constructs the full instruction (including expensive diff computation) just to extract pubkeys. Consider adding a lightweight account_pubkeys() method that returns the required pubkeys without building args or computing diffs.
Based on past review comments.
🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/mod.rs around lines 65-66,
involved_accounts currently constructs the full Instruction (including expensive
args/diff computation) just to extract pubkeys; implement a lightweight method
(e.g., Instruction::account_pubkeys() or similar on the type that represents the
instruction descriptor) that returns only the necessary pubkeys without building
args or computing diffs, then change involved_accounts() to call that new
method; ensure the new method reads from the instruction metadata
(accounts/keys) only, does not allocate heavy structures, and update
callers/tests/benchmarks accordingly.
4ea5450 to
41493c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test-integration/programs/schedulecommit/src/api.rs (1)
100-145: Validateuser_seedto prevent creating instructions with invalid PDAs.The current implementation accepts arbitrary
user_seedvalues and the conditional at line 127 only checks forb"magic_schedule_commit", treating everything else as an OrderBook delegation. This creates two problems:
- Invalid seeds (e.g.,
b"invalid_seed") will produce instructions with PDAs that don't match on-chain expectations, causing runtime failures.- The PDA derivation at lines 107-110 uses the arbitrary seed, but the on-chain handlers expect canonical seeds:
[b"magic_schedule_commit", player]or[b"order_book", book_manager].Consider adding validation:
pub fn delegate_account_cpi_instruction( payer: Pubkey, validator: Option<Pubkey>, player_or_book_manager: Pubkey, user_seed: &[u8], ) -> Instruction { + // Validate seed is one of the expected values + let is_main_account = user_seed == b"magic_schedule_commit"; + let is_order_book = user_seed == b"order_book"; + + if !is_main_account && !is_order_book { + panic!("Invalid user_seed: must be either 'magic_schedule_commit' or 'order_book'"); + } + let program_id = crate::id(); let (pda, _) = Pubkey::find_program_address( &[user_seed, player_or_book_manager.as_ref()], &crate::ID, ); // ... rest of the function Instruction::new_with_borsh( program_id, - &if user_seed == b"magic_schedule_commit" { + &if is_main_account { ScheduleCommitInstruction::DelegateCpi(DelegateCpiArgs { valid_until: i64::MAX, commit_frequency_ms: 1_000_000_000, player: player_or_book_manager, validator, }) } else { + // is_order_book is true here due to earlier validation ScheduleCommitInstruction::DelegateOrderBook( DelegateOrderBookArgs { commit_frequency_ms: 1_000_000_000, book_manager: player_or_book_manager, validator, }, ) }, account_metas, ) }Alternatively, use a typed enum parameter instead of raw
&[u8]to enforce correctness at the type level.magicblock-committor-service/src/tasks/mod.rs (1)
420-438: Critical: Test function must be async.The test calls
CommitTask::new(...).await(lines 424-437), but the function is marked with#[test]instead of#[tokio::test]. This will cause a compilation error because.awaitcannot be used in a non-async function.Apply this diff:
-#[test] -fn test_args_task_instruction_serialization() { +#[tokio::test] +async fn test_args_task_instruction_serialization() { let validator = Pubkey::new_unique(); // Test Commit variant - let commit_task: ArgsTask = ArgsTaskType::Commit(CommitTask::new( + let commit_task: ArgsTask = ArgsTaskType::Commit(CommitTask::new( 123, true, CommittedAccount { pubkey: Pubkey::new_unique(), account: Account { lamports: 1000, data: vec![1, 2, 3], owner: Pubkey::new_unique(), executable: false, rent_epoch: 0, }, }, + AccountFetcher::new(), - )) + ).await) .into();
♻️ Duplicate comments (7)
test-integration/programs/schedulecommit/src/lib.rs (3)
278-309: Critical: OrderBook header must be initialized to prevent undefined behavior.After
allocate_account_and_assign_ownercompletes, the account data contains uninitialized memory. The OrderBook header fields (bids_len,asks_len) will have arbitrary values, causing subsequent operations to read invalid counts and potentially access out-of-bounds memory.Apply this fix to zero-initialize the header:
allocate_account_and_assign_owner(AllocateAndAssignAccountArgs { payer_info: payer, account_info: order_book, owner: &crate::ID, signer_seeds: &[b"order_book", book_manager.key.as_ref(), &[bump]], size: 10 * 1024, })?; + // Initialize header to zero + let mut data = order_book.try_borrow_mut_data()?; + let header_size = core::mem::size_of::<order_book::OrderBookHeader>(); + if data.len() < header_size { + return Err(ProgramError::AccountDataTooSmall); + } + // Zero the header bytes + data[..header_size].fill(0); Ok(())This ensures
bids_lenandasks_lenstart at 0, making the OrderBook safe to use.
327-332: Minor: Error message references wrong key.The error message at line 330 uses
payer.keybut should usebook_manager.keyto accurately report which PDA validation failed.Apply this fix:
assert_keys_equal(order_book.key, &pda, || { format!( "PDA for the account ('{}') and for book_manager ('{}') is incorrect", - order_book.key, payer.key + order_book.key, book_manager.key ) })?;
334-350: Critical: Guard against integer overflow in size calculation.The unchecked addition
order_book.data_len() + additional_space as usizecan overflow on BPF release builds. Ifadditional_spaceis large (e.g., close tousize::MAX), the sum wraps to a small value, causingreallocto shrink the account and truncate the OrderBook data.Apply this fix to use checked arithmetic:
- let new_size = order_book.data_len() + additional_space as usize; + let additional = additional_space as usize; + let new_size = order_book + .data_len() + .checked_add(additional) + .ok_or_else(|| { + msg!("Size overflow: current={} + additional={}", order_book.data_len(), additional); + ProgramError::InvalidArgument + })?;Also consider adding an upper bound check (e.g.,
MAX_ORDER_BOOK_SIZE) to prevent extremely large allocations.magicblock-committor-service/src/tasks/task_builder.rs (1)
90-102: Duplicate: AccountFetcher instantiated per account.Creating
AccountFetcher::new()inside the map for every account remains inefficient, as each instantiation reparses configuration and creates new RPC clients. The architectural feedback in PR comments recommends moving account-fetching logic out of tasks entirely intoTaskInfoFetcher. Per the author's response, this is acknowledged as temporary and will be addressed in follow-up PRs.Based on past review comments and PR objectives.
magicblock-committor-service/src/tasks/mod.rs (3)
115-117: Duplicate: Consider separate task types instead of optional fields.The
base_account: Option<Account>andforce_commit_state: boolfields support runtime selection between diff and state paths. However, as noted in past reviews, separate task types (e.g.,CommitTaskandCommitWithDiffTask) would eliminate these optional fields and clarify the commit path at construction time. Per PR discussion, this architectural refactor is acknowledged for future work.Based on past review comments.
120-122: Duplicate: Document COMMIT_STATE_SIZE_THRESHOLD rationale.The 200-byte threshold determines when to use
CommitDiff(RPC-based, smaller instruction data) versusCommitState(direct, larger instruction data). Consider adding a comment explaining why 200 bytes was chosen, the trade-offs (RPC overhead vs instruction size), and whether this should be configurable.Based on past review comments.
124-155: Duplicate: RPC logic should move out of task constructor.The async constructor performs RPC account fetching directly (lines 133-143), which places network/external responsibilities inside the task. Per reviewer feedback (taco-paco), tasks should represent on-chain instructions and should not fetch account data; that logic belongs in
TaskInfoFetcher. The author has acknowledged this as a temporary workaround to be addressed in follow-up PRs.The error handling improvement (lines 138-142) that logs fetch failures is good and addresses past feedback.
Based on past review comments and PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-committor-service/src/tasks/mod.rs(8 hunks)magicblock-committor-service/src/tasks/task_builder.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(5 hunks)test-integration/programs/schedulecommit/src/lib.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/programs/schedulecommit/src/api.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (4)
magicblock-committor-service/src/tasks/task_builder.rs (2)
magicblock-committor-service/src/tasks/mod.rs (1)
new(124-155)magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)
test-integration/programs/schedulecommit/src/api.rs (1)
test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
magicblock-committor-service/src/tasks/mod.rs (1)
magicblock-committor-service/src/tasks/account_fetcher.rs (1)
new(14-47)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/programs/schedulecommit/src/utils/mod.rs (3)
assert_is_signer(30-44)assert_keys_equal(16-28)allocate_account_and_assign_owner(58-106)test-integration/programs/schedulecommit/src/order_book.rs (1)
new(105-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (15)
test-integration/programs/schedulecommit/src/api.rs (4)
36-54: LGTM: OrderBook initialization instruction properly structured.The account metas and instruction construction follow the established pattern. The
book_manageris correctly marked as readonly signer, and theorder_bookaccount is writable.
56-75: LGTM: GrowOrderBook instruction structure is correct.Note that
book_manageris marked as non-signer here (line 65), unlike ininit_order_book_instructionwhere it's a signer. This is consistent with the on-chain handler which only validates the PDA relationship, not the manager's signature.
178-194: LGTM: UpdateOrderBook instruction is correctly structured.The account metas and instruction payload are properly constructed.
196-215: LGTM: ScheduleCommitForOrderBook instruction is correctly constructed.The account metas match the on-chain handler expectations, with proper signer and writable flags.
test-integration/programs/schedulecommit/src/lib.rs (9)
14-19: LGTM: New imports support OrderBook growth and system interactions.The additions of
invoke,Rent,system_instruction, andSysvarare necessary for the newprocess_grow_order_bookfunction to transfer lamports and compute rent requirements.
31-35: LGTM: OrderBook module integration is clean.The private module with selective public re-exports follows Rust best practices.
50-55: LGTM: DelegateOrderBookArgs struct follows established pattern.The structure mirrors
DelegateCpiArgswith appropriate fields for OrderBook delegation.
123-136: LGTM: New instruction variants are well-defined.The OrderBook-related instruction variants follow the established pattern and include appropriate parameter types.
179-187: LGTM: Instruction dispatch correctly routes OrderBook operations.All new instruction variants are properly wired to their respective handler functions.
201-201: LGTM: SIZE type changed to u64 for compatibility.Changing from
usizetou64ensures the constant is compatible with Solana's account size APIs and cross-platform serialization.
358-391: LGTM: OrderBook delegation follows the established pattern.The function correctly constructs seeds with
b"order_book"andbook_manager, and properly invokesdelegate_accountwith all required accounts and configuration.
396-412: LGTM: UpdateOrderBook handler is correctly implemented.The function properly validates the signer, borrows account data mutably, and applies updates via the OrderBook API. The log message is accurate.
417-437: LGTM: ScheduleCommitForOrderBook handler is correctly implemented.The function properly:
- Returns
NotEnoughAccountKeysfor missing accounts (line 424)- Validates
payeris a signer (line 427)- Invokes the commit-and-undelegate flow with correct account ordering
magicblock-committor-service/src/tasks/task_builder.rs (1)
4-4: LGTM: Imports support async task construction.The addition of
join_allandAccountFetcherappropriately supports the new concurrent async task creation pattern.Also applies to: 12-12
magicblock-committor-service/src/tasks/mod.rs (1)
1-5: LGTM: Imports correctly support CommitDiff functionality.The new imports for
account_fetcher, DLP args (CommitDiffArgs,CommitStateArgs),compute_diff, andsolana_accounttypes are necessary and properly used throughout the file. The previously flagged unused imports have been removed.Also applies to: 21-21, 36-36
| pub fn is_commit_diff(&self) -> bool { | ||
| !self.force_commit_state | ||
| && self.committed_account.account.data.len() | ||
| > CommitTask::COMMIT_STATE_SIZE_THRESHOLD | ||
| && self.base_account.is_some() | ||
| } | ||
|
|
||
| pub fn force_commit_state(&mut self) { | ||
| self.force_commit_state = true; | ||
| } | ||
|
|
||
| pub fn create_commit_ix(&self, validator: &Pubkey) -> Instruction { | ||
| if let Some(fetched_account) = self.base_account.as_ref() { | ||
| self.create_commit_diff_ix(validator, fetched_account) | ||
| } else { | ||
| self.create_commit_state_ix(validator) | ||
| } | ||
| } | ||
|
|
||
| fn create_commit_state_ix(&self, validator: &Pubkey) -> Instruction { | ||
| let args = CommitStateArgs { | ||
| nonce: self.commit_id, | ||
| lamports: self.committed_account.account.lamports, | ||
| data: self.committed_account.account.data.clone(), | ||
| allow_undelegation: self.allow_undelegation, | ||
| }; | ||
| dlp::instruction_builder::commit_state( | ||
| *validator, | ||
| self.committed_account.pubkey, | ||
| self.committed_account.account.owner, | ||
| args, | ||
| ) | ||
| } | ||
|
|
||
| fn create_commit_diff_ix( | ||
| &self, | ||
| validator: &Pubkey, | ||
| fetched_account: &Account, | ||
| ) -> Instruction { | ||
| if self.force_commit_state { | ||
| return self.create_commit_state_ix(validator); | ||
| } | ||
|
|
||
| let args = CommitDiffArgs { | ||
| nonce: self.commit_id, | ||
| lamports: self.committed_account.account.lamports, | ||
| diff: compute_diff( | ||
| fetched_account.data(), | ||
| self.committed_account.account.data(), | ||
| ) | ||
| .to_vec(), | ||
| allow_undelegation: self.allow_undelegation, | ||
| }; | ||
|
|
||
| dlp::instruction_builder::commit_diff( | ||
| *validator, | ||
| self.committed_account.pubkey, | ||
| self.committed_account.account.owner, | ||
| args, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Remove redundant force_commit_state check.
The create_commit_diff_ix method checks self.force_commit_state at lines 196-198 and falls back to create_commit_state_ix. However, this is redundant because create_commit_ix (line 169) only calls create_commit_diff_ix when base_account is Some, and the constructor ensures base_account is Some only when not forcing state. The check is defensive but unnecessary.
Otherwise, the diff/state instruction building logic is correct and well-structured.
🤖 Prompt for AI Agents
In magicblock-committor-service/src/tasks/mod.rs around lines 157 to 217, remove
the redundant defensive check of self.force_commit_state inside
create_commit_diff_ix (lines ~196-198) because create_commit_ix only calls
create_commit_diff_ix when base_account is Some and the constructor guarantees
base_account is Some only when not forcing state; simply build and return the
CommitDiffArgs and call dlp::instruction_builder::commit_diff without the early
return to create_commit_state_ix, ensuring no other behavioral changes.
| #[tokio::test] | ||
| async fn test_buffer_task_instruction_serialization() { | ||
| let validator = Pubkey::new_unique(); | ||
|
|
||
| let buffer_task = BufferTask::new_preparation_required( | ||
| BufferTaskType::Commit(CommitTask { | ||
| commit_id: 456, | ||
| allow_undelegation: false, | ||
| committed_account: CommittedAccount { | ||
| pubkey: Pubkey::new_unique(), | ||
| account: Account { | ||
| lamports: 2000, | ||
| data: vec![7, 8, 9], | ||
| owner: Pubkey::new_unique(), | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| let buffer_task = | ||
| BufferTask::new_preparation_required(BufferTaskType::Commit( | ||
| CommitTask::new( | ||
| 456, | ||
| false, | ||
| CommittedAccount { | ||
| pubkey: Pubkey::new_unique(), | ||
| account: Account { | ||
| lamports: 2000, | ||
| data: vec![7, 8, 9], | ||
| owner: Pubkey::new_unique(), | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| ); | ||
| AccountFetcher::new(), | ||
| ) | ||
| .await, | ||
| )); | ||
| assert_serializable(&buffer_task.instruction(&validator)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Should these tests use multi_thread runtime?
These async tests properly call CommitTask::new(...).await with AccountFetcher::new(). However, per the PR objectives, some tests were changed to #[tokio::test(flavor = "multi_thread", worker_threads = 2)] because AccountFetcher::new() uses blocking RpcClient calls that require a multi-threaded runtime.
Please verify whether these two tests should also use the multi_thread flavor:
test_buffer_task_instruction_serialization(line 479)test_preparation_instructions_serialization(line 506)
If AccountFetcher::new() performs blocking RPC calls, these tests may hang or fail on a single-threaded runtime.
Also applies to: 506-543
🏁 Script executed:
#!/bin/bash
# Search for other tests using multi_thread flavor with AccountFetcher
rg -n --type=rust '#\[tokio::test\(flavor.*multi_thread' -A 10 | rg -C 5 'AccountFetcher::new'Length of output: 0
🏁 Script executed:
#!/bin/bash
# 1. Find the file and show tests at the specified lines
echo "=== Tests at lines 479-543 ==="
fd -e rs 'mod\.rs' -path '*committor-service/src/tasks*' -exec cat {} \; | sed -n '479,543p'Length of output: 199
🏁 Script executed:
#!/bin/bash
# 2. Search for AccountFetcher struct/impl to understand if new() is blocking
echo "=== AccountFetcher definition and new() method ==="
rg -n 'impl AccountFetcher' -A 20 --type=rust | head -50Length of output: 2099
🏁 Script executed:
#!/bin/bash
# 3. Check if any tests in this file already use multi_thread
echo "=== Tests with multi_thread flavor in the codebase ==="
rg -n 'tokio::test.*multi_thread' --type=rust -B 2 -A 5Length of output: 33066
🏁 Script executed:
#!/bin/bash
# 4. Check what tests currently exist in the mod.rs file
echo "=== All #[tokio::test] definitions in the file ==="
fd -e rs 'mod\.rs' -path '*committor-service/src/tasks*' -exec rg -n '#\[tokio::test' {} \;Length of output: 219
🏁 Script executed:
#!/bin/bash
# Find the exact file path and show lines 479-543
find . -name mod.rs -path '*/committor-service/src/tasks/*' 2>/dev/null | head -1 | xargs -I {} sh -c 'echo "File: {}"; sed -n "479,543p" {}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at CommitTask::new() to understand if it's blocking
rg -n 'impl CommitTask' -A 30 | head -60Length of output: 2640
🏁 Script executed:
#!/bin/bash
# Search for comments about blocking RPC in CommitTask context
rg -n 'CommitTask::new' -B 2 -A 2 --type-add 'rust:*.rs' --type rust | grep -C 3 'blocking\|await' | head -40Length of output: 3639
🏁 Script executed:
#!/bin/bash
# Show the actual tests at lines 479-543 using rg with line numbers
rg -n . magicblock-committor-service/src/tasks/mod.rs | awk -F: '$1 >= 479 && $1 <= 543 {print}'Length of output: 2604
🏁 Script executed:
#!/bin/bash
# Check fetch_account implementation to understand what it does
rg -n 'fn fetch_account' -A 10 magicblock-committor-service/src/tasks/Length of output: 627
🏁 Script executed:
#!/bin/bash
# Verify if these specific tests in mod.rs already use multi_thread
rg -n 'test_buffer_task_instruction_serialization|test_preparation_instructions_serialization' -B 5 magicblock-committor-service/src/tasks/mod.rsLength of output: 426
Clarify which test needs multi_thread runtime—and correct the blocking RPC source
The original concern contains inaccuracies. The blocking RPC call is in CommitTask::new() (via fetch_account()), not in AccountFetcher::new(). Additionally:
-
test_buffer_task_instruction_serialization(line 479) uses 3 bytes of data, which is below the 200-byte threshold (COMMIT_STATE_SIZE_THRESHOLD), sofetch_account()is never called. No blocking RPC occurs; default#[tokio::test]is appropriate. -
test_preparation_instructions_serialization(line 506) uses 1024 bytes of data, which exceeds the threshold, sofetch_account()will be called and blocking RPC will occur. This test should use#[tokio::test(flavor = "multi_thread", worker_threads = 2)]per the established codebase pattern.
Apply multi_thread flavor to line 506 only.
🤖 Prompt for AI Agents
magicblock-committor-service/src/tasks/mod.rs around lines 479 to 503: the
blocking RPC call happens inside CommitTask::new() (via fetch_account()) and
only the test that triggers fetch_account() needs the multi_thread runtime;
update the test at line ~506 (test_preparation_instructions_serialization) to
use #[tokio::test(flavor = "multi_thread", worker_threads = 2)] instead of the
default #[tokio::test], leaving test_buffer_task_instruction_serialization (line
479) unchanged because its 3-byte data does not hit the
COMMIT_STATE_SIZE_THRESHOLD and does not call fetch_account().
This PR does the following things:
CommitDiff. So now when a commit is scheduled, it triggers eitherCommitStateorCommitDiffin the delegation-program, depending on the account size and theCOMMIT_STATE_SIZE_THRESHOLD.CommitDiffis effectively an optimization that reduces the amount of data transferred when committing small changes in large accounts, which improves performance and reduces costs.args, notbufferaccount.OrderBookaccount (10 KB). I initially wanted to try a few MB, but realized currently that’s not possible yet.CommitDiffwas invoked (in the delegation-program) with onlydiff len = 286whereasdata len = 10240(see first screenshot).diff len = 8(encoding the size of account on ER and the number of offset-pairs), and logs a warning (see second screenshot).Update dlp.so
Updated
test-integration/schedulecommit/elfs/dlp.soto use latest version of DLP withCommitDiffix.Changes in the existing tests.
Two kinds of changes:
Since it is
CommitTaskwhich decides the exact DLP instructionCommitStateorCommitDiff, and many tests which earlier used to get executed asBufferTask, will now be executed asArgsTaskbecause large accounts with small changes can be executed asArgsTaskas long as the transaction size stays within limit. So made the related changes in the tests.For some tests, I made the following change:
It is because now
CommitTask::new()uses blockingRpcClientto fetch account from the base chain, so the tests fail because they run in a single-threaded environment, giving this error:Using
multi_threadwith 2 threads makes the tests work again.Screenshots
Summary by CodeRabbit
New Features
API
Tests
Chores